test: lock client-router query-string preservation on every nav path - #926
Conversation
…639) Query params sometimes appeared not to carry forward while dogfooding the blog. A source review found the router mostly correct (finalUrl defaults to the full href, the snapshot/prefetch cache keys on pathname+search, the GET-form reset is spec-compliant), so this is primarily locked browser coverage that also surfaces any genuine drop. Adds packages/core/test/routing/browser/query-params.test.js: a link with a query preserves location.search + the fetched URL; sequential navs keep distinct params (not merged, not dropped); the prefetch cache is keyed by search; back/forward restores the prior entry query; a GET-form submission replaces the action query with the form fields (spec-correct, asserted so it is not mistaken for a bug); a redirect updates the URL to the target query. Passes on Chromium, Firefox, and WebKit, so no genuine drop exists. Closes #639
|
Counterfactual proof: the lock has teeth (all three router mechanisms) Since this is a coverage-lock PR, I confirmed each assertion actually fails when the router line it guards is broken. Three source reverts cover all six cases (each run on Chromium + Firefox + WebKit):
So the six cases genuinely exercise the three distinct query paths (history URL, cache key, form-submission reset), not just a happy path. No source change was needed: the router already preserves query strings correctly on every path, which is what the review found and this pins. |
Cross-referenced Next.js and Remix client-router query-string test suites for edge cases worth locking: - repeated keys (?foo=bar&foo=baz) are a DISTINCT cache key from the single-value form. Next had a real bug here (#92787): its key collapsed repeated keys via Object.fromEntries, keeping only the last value, so a multi-value to single-value nav was a false cache hit. WebJs keys on the raw pathname+search string, so it does not collapse; lock that. - a hash and a query survive a nav together, and a hash change does not drop the query. - an encoded / unicode query value round-trips through the nav. All nine cases pass on Chromium, Firefox, and WebKit.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Checked each case against the router source to make sure none pass vacuously. The prefetch-keying case genuinely populates the cache (swapBody has a text/html 200 so it reaches prefetchStore) and the ?x=2 / query-less peeks miss only because cacheKey is pathname+search. The back/forward case asserts ?y=2 before history.back() so the ?x=1 restore is a real popstate observation, not a pre-set value. The redirect case's defineProperty shadows the Response getters and drives the finalUrl=resp.url branch. Isolation holds: teardown restores fetch + replaceState(before) + _resetPrefetch, and each test pushes its own entries before going back. The GET-form action really carries ?old=1 so the replace assertion has teeth. Nice and tight.
Replace the single fixed 25ms delay with a poll for the routed fetch, so the GET-form case cannot flake on a slow CI runner. The other cases already await navigate() to completion; this brings the one click-driven case in line.
The prior commit polled for the routed fetch, which is recorded BEFORE the pushState that sets location.search, so the location.search assertion ran too early and failed. Poll location.search (which settles last) instead, so the whole submission chain has run before the assertion.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Reworked the GET-form case to poll location.search (which settles after the fetch -> swap -> pushState chain) instead of a fixed delay, so it can't flake on a slow runner and the fetched-call lookup is guaranteed present by the time search settles. Re-ran the whole suite several times across the three engines, 9/9 every time. Nothing else moved.
Turbo's form_submission_tests assert a GET form does not incorporate the current page's URLSearchParams into the submission (only the form fields). WebJs behaves the same (the absolute action path replaces the search before the fields are set); lock it so WebJs stays on par with the Turbo router it derives from. 10 cases, all green on Chromium/Firefox/WebKit.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Added the Turbo GET-form parity case (a GET form must not merge the current page's query into the submission) after cross-checking against the Turbo router this is derived from. It resolves the absolute action path against location.href, which drops the page's ?sort=old before the form fields set the search, so ?q=new wins. The find() filters on q=new so it doesn't pick up the earlier navigate() fetch of the same path. Ten cases now, stable across the three engines.
1 participant
Closes #639
Query params sometimes appeared NOT to carry forward while dogfooding the blog. A source review of
packages/core/src/router-client.jsfound the router mostly correct:finalUrldefaults to the full linkhref(sopushStatekeeps the query), the snapshot + prefetch cache key onpathname + search(so?x=1and?x=2are distinct), and the GET-formurl.search = ''rebuild is the spec-compliant HTML5 behaviour. So this is primarily locked browser coverage that will ALSO surface any genuine drop.What it adds
packages/core/test/routing/browser/query-params.test.js(real browser, all three wtr engines):location.searchAND in the URL the router fetches;/a?x=1then/b?y=2lands?y=2, not merged, not dropped);?foo=bar&foo=bazstay a DISTINCT cache key from the single-value form (WebJs keys on the rawpathname+search, so it does not collapse them the way Next's #92787 bug did); a hash + a query survive a nav together and a hash change does not drop the query; an encoded / unicode query value round-trips.Result: all nine cases pass on Chromium, Firefox, and WebKit, so no genuine query-string drop exists in the current router. No source change was needed.
Definition-of-done surfaces
agent-docs/advanced.md("Form submissions", the GET bullet).packages/*/src, no generated-app or editor surface).Test plan